@relayburn/sdk: npm umbrella + napi-rs CI matrix + conformance scaffold (#247 part b)#308
Merged
willwashburn merged 7 commits intomainfrom May 6, 2026
Merged
Conversation
…ce gate Wave 1 D2 of the Rust port (#247 part b). Stands up the npm packaging side of the napi-rs deliverable while #247-a (parallel agent) fills in crates/relayburn-sdk-node. - packages/sdk-node/: umbrella @relayburn/sdk@2.0.0-pre.0 with thin TS facade re-exporting the napi-rs verbs (ingest, summary, sessionCost, overhead, overheadTrim, hotspots, compare). Resolves the right native binary via optionalDependencies on @relayburn/sdk-{darwin-arm64, darwin-x64,linux-arm64-gnu,linux-x64-gnu}. win32-x64-msvc deferred to a #247 follow-up. - pnpm-workspace.yaml: exclude packages/sdk-node so the 2.x umbrella doesn't collide with the 1.x @relayburn/sdk during workspace:* resolution. CI installs sdk-node deps with --ignore-workspace. - .github/workflows/napi-build.yml: napi-rs build matrix (darwin-arm64, darwin-x64, linux-arm64-gnu, linux-x64-gnu) on PRs / main / dispatch. Caches Cargo + pnpm. Publish step is stubbed; full wiring lands in #249 (lockstep release workflow). - packages/sdk-node/test/conformance.test.js: deep-equal scaffold comparing TS @relayburn/sdk@1.x vs the napi-rs umbrella across the six verbs. Skips while #247-a is in flight; flip RELAYBURN_SDK_NAPI_BUILT=1 in CI once bindings land. - packages/sdk-node/test/esbuild-smoke.test.js: bundles the umbrella facade through esbuild with native-binding externals — catches resolution / bundling regressions for downstream embedders. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Node.js N-API v2 SDK under ChangesNode SDK v2.x with N-API Bindings & CI
Sequence Diagram(s)sequenceDiagram
autonumber
actor "GitHub Actions" as GH
participant "Runner (macOS/Linux)" as Runner
participant "pnpm / Node" as NodeTool
participant "Rust / cross" as RustTool
participant "Artifacts (upload)" as Artifacts
participant "Tests (esbuild/conformance)" as Tests
participant "Publish Job (stub)" as Publish
GH->>Runner: trigger workflow (push/pr/dispatch)
Runner->>NodeTool: setup pnpm & Node
Runner->>RustTool: setup Rust toolchain (cross for linux-arm64)
Runner->>Runner: run matrix builds (per-target napi build)
Runner->>Artifacts: produce and upload .node artifacts
Runner->>Tests: run esbuild smoke (conformance skipped if bindings missing)
alt workflow_dispatch publish=true
Publish->>Artifacts: download artifacts
Publish->>Publish: execute stubbed publish steps
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54d0f7fdf1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+66
to
+68
| if (existsSync(FIXTURE_LEDGER_DIR)) { | ||
| cpSync(FIXTURE_LEDGER_DIR, home, { recursive: true }); | ||
| } |
There was a problem hiding this comment.
This test gate silently falls back to an empty temporary ledgerHome whenever tests/fixtures/ledger is absent, so once RELAYBURN_SDK_NAPI_BUILT=1 is enabled the deep-equality checks can pass on near-empty outputs instead of real fixture data. In this repo there is no tests/fixtures/ledger tree, so the current behavior weakens the core parity check and can let Rust/TS divergences ship undetected.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Addressed in 7801bd9. Removed the empty-tempdir fallback in makeHome() and added an ensureFixtureLedger() precondition at the top of callBoth() that throws (with a seed hint) when tests/fixtures/ledger/ is missing. With RELAYBURN_SDK_NAPI_BUILT=1 and no fixture, all 7 conformance tests now fail loudly instead of tautologically passing on empty homes; with the env var unset the suite still skips. Documented the fixture location + seeding expectation in the file header so the gate can't be silently re-neutered.
Comment on lines
+93
to
+97
| const VERBS = [ | ||
| { name: 'summary', opts: {} }, | ||
| { name: 'sessionCost', opts: { session: 'fixture-session-1' } }, | ||
| { name: 'overhead', opts: { project: REPO_ROOT } }, | ||
| { name: 'overheadTrim', opts: { project: REPO_ROOT, includeDiff: false } }, |
There was a problem hiding this comment.
The facade exports compare, but the conformance matrix only enumerates summary/sessionCost/overhead/overheadTrim/hotspots (with ingest separate), so behavioral drift in compare() will not be caught by the parity gate. Since 2.x is presented as a drop-in replacement, leaving this exported verb unverified creates a test hole in a user-facing API.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Addressed in 7801bd9. Added a compare row to the VERBS matrix with { models: ['claude-sonnet-4-5', 'claude-opus-4-7'], minFidelity: 'partial' } — both implementations see the same fixture so even rows with no matches surface as identical empty/no-data shapes, while drift in cell layout, fidelity accounting, or totals still fails the deep-equal. Updated the file-header verb count from 6 to 7 to match.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/sdk-node/CHANGELOG.md (1)
5-9: 💤 Low valueRemove the issue/PR link from the changelog entry.
The coding guidelines specify to drop issue/PR links from package changelog entries. Remove
(#247part b)from the end of this entry.Suggested fix
- Initial scaffolding: umbrella package layout (`@relayburn/sdk`) + per-platform packages (`@relayburn/sdk-{darwin-arm64,darwin-x64,linux-arm64-gnu,linux-x64-gnu}`) resolved via `optionalDependencies`, TS facade re-exporting the napi-rs - binding, conformance scaffold against the TS 1.x SDK, esbuild bundle - smoke test. (`#247` part b) + binding, conformance scaffold against the TS 1.x SDK, esbuild bundle + smoke test.As per coding guidelines: "drop issue/PR links, internal notes, backstory".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk-node/CHANGELOG.md` around lines 5 - 9, Edit the CHANGELOG.md entry that begins "Initial scaffolding: umbrella package layout..." and remove the trailing issue/PR reference string "(`#247` part b)" so the line ends after "smoke test."; ensure no other text is altered and commit the cleaned changelog entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/napi-build.yml:
- Around line 52-54: The workflow currently uses the retired runner label "os:
macos-13" for the x86_64-apple-darwin matrix (see the block containing "target:
x86_64-apple-darwin" and "short: darwin-x64"); update that runner label to a
supported Intel macOS image such as "macos-15-intel" (or "macos-14-intel") so
the CI job continues to run, leaving the other keys ("target" and "short")
unchanged.
In `@packages/sdk-node/src/index.cjs`:
- Line 8: Rename the CommonJS export file binding.js to binding.cjs and update
all requires to the new filename: change require('./binding.js') to
require('./binding.cjs') in both index.cjs and index.js; also update the
package.json "files" array (and any build/generation scripts) to reference
binding.cjs instead of binding.js so consumers see the correct CommonJS
artifact. Ensure the renamed file preserves module.exports and that
index.cjs/index.js still use require(...) (not import) to load binding.cjs.
In `@packages/sdk-node/test/conformance.test.js`:
- Around line 118-132: The test comment promises a follow-up summary()
comparison but the code only compares the ingest return value; either update the
comment to remove the summary claim or implement the summary check: after
callBoth('ingest', {}) use callBoth('summary', {}) (same helper used in this
file) to get summary results and assert.deepStrictEqual(summary.napiResult,
summary.tsResult) to confirm both implementations wrote the same rows; update
the test block for test 'conformance: ingest() matches TS 1.x' and reference
callBoth, out.napiResult/out.tsResult, and the summary results when making the
change.
---
Nitpick comments:
In `@packages/sdk-node/CHANGELOG.md`:
- Around line 5-9: Edit the CHANGELOG.md entry that begins "Initial scaffolding:
umbrella package layout..." and remove the trailing issue/PR reference string
"(`#247` part b)" so the line ends after "smoke test."; ensure no other text is
altered and commit the cleaned changelog entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ac7f7d98-57ad-417e-b118-32e869b5fa51
⛔ Files ignored due to path filters (1)
packages/sdk-node/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.github/workflows/napi-build.ymlCHANGELOG.mdpackages/sdk-node/CHANGELOG.mdpackages/sdk-node/README.mdpackages/sdk-node/npm/darwin-arm64/README.mdpackages/sdk-node/npm/darwin-arm64/package.jsonpackages/sdk-node/npm/darwin-x64/README.mdpackages/sdk-node/npm/darwin-x64/package.jsonpackages/sdk-node/npm/linux-arm64-gnu/README.mdpackages/sdk-node/npm/linux-arm64-gnu/package.jsonpackages/sdk-node/npm/linux-x64-gnu/README.mdpackages/sdk-node/npm/linux-x64-gnu/package.jsonpackages/sdk-node/package.jsonpackages/sdk-node/src/binding.d.tspackages/sdk-node/src/binding.jspackages/sdk-node/src/index.cjspackages/sdk-node/src/index.d.tspackages/sdk-node/src/index.jspackages/sdk-node/test/conformance.test.jspackages/sdk-node/test/esbuild-smoke.test.jspnpm-workspace.yaml
Address review feedback on PR #308: - Fail conformance fast when `RELAYBURN_SDK_NAPI_BUILT=1` is set but the fixture ledger at `tests/fixtures/ledger/` is missing. Previously the test silently fell back to an empty temp home, which would let the deep-equality checks pass on near-empty outputs once the gate was enabled. - Add `compare()` to the conformance verb matrix so behavioral drift in the per-(model, activity) shape is caught alongside the other read verbs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/sdk-node/test/conformance.test.js`:
- Around line 75-83: ensureFixtureLedger currently only verifies
FIXTURE_LEDGER_DIR exists, allowing empty or mis-copied fixtures to pass; update
ensureFixtureLedger to validate the fixture contents (e.g., require a small
manifest or specific expected filenames and that those files are non-empty or
exceed a minimal size). In practice, check FIXTURE_LEDGER_DIR for a required set
of files (use the existing FIXTURE_LEDGER_DIR constant and the
ensureFixtureLedger() function), verify each required file exists and is not
zero bytes (or validate a simple checksum/manifest if present), and throw a
descriptive Error if any file is missing or malformed so the conformance suite
fails closed on bad fixtures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d0733ced-edc4-4e3d-98c6-a2a532993575
📒 Files selected for processing (1)
packages/sdk-node/test/conformance.test.js
… (review fixes round 2)
- Rename src/binding.js to src/binding.cjs so Node can require it under
the package's "type": "module" setting (ESM would reject module.exports
on a bare .js). Update src/index.{js,cjs} requires, the package.json
files array + build:napi script, and the napi-build workflow's
--js flag to match. Add a binding.cjs header note explaining the
extension choice so a future napi build regen doesn't accidentally
flip back to .js.
- Bump the x86_64-apple-darwin matrix runner from macos-13 (retired
Dec 2025) to macos-15-intel.
- Fix the ingest() conformance test comment: the "follow-up summary()
comparison" was aspirational, not implemented; reword as a
forward-looking note instead of describing absent behavior.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/sdk-node/test/conformance.test.js (1)
75-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden fixture validation to prevent silent false-positives.
The current check only verifies the directory exists. An empty or malformed fixture would let both implementations return identical empty results, causing the deep-equality checks to pass vacuously.
Proposed hardening
+import { statSync } from 'node:fs'; ... function ensureFixtureLedger() { + const ledgerJsonl = join(FIXTURE_LEDGER_DIR, 'ledger.jsonl'); if (!existsSync(FIXTURE_LEDGER_DIR)) { throw new Error( `conformance fixture ledger missing at ${FIXTURE_LEDGER_DIR}. ` + `Seed it (e.g. cp -R ~/.relayburn tests/fixtures/ledger) before ` + `running with RELAYBURN_SDK_NAPI_BUILT=1, or unset the env var to ` + `skip the conformance suite.`, ); } + if (!existsSync(ledgerJsonl) || statSync(ledgerJsonl).size === 0) { + throw new Error( + `conformance fixture is incomplete: expected a non-empty ${ledgerJsonl}`, + ); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk-node/test/conformance.test.js` around lines 75 - 84, The ensureFixtureLedger function currently only checks for existence of FIXTURE_LEDGER_DIR; update it to validate the fixture contents to avoid empty/malformed fixtures by checking for required files and non-empty data: inside ensureFixtureLedger validate that FIXTURE_LEDGER_DIR contains expected files (e.g., any ledger file names your tests rely on) and that those files are non-empty (size > 0) and/or parseable (e.g., attempt a lightweight JSON/format parse if applicable); if any required file is missing, empty, or invalid, throw an Error with a clear message referencing FIXTURE_LEDGER_DIR so the conformance suite fails fast rather than producing vacuous equality results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/sdk-node/test/conformance.test.js`:
- Around line 75-84: The ensureFixtureLedger function currently only checks for
existence of FIXTURE_LEDGER_DIR; update it to validate the fixture contents to
avoid empty/malformed fixtures by checking for required files and non-empty
data: inside ensureFixtureLedger validate that FIXTURE_LEDGER_DIR contains
expected files (e.g., any ledger file names your tests rely on) and that those
files are non-empty (size > 0) and/or parseable (e.g., attempt a lightweight
JSON/format parse if applicable); if any required file is missing, empty, or
invalid, throw an Error with a clear message referencing FIXTURE_LEDGER_DIR so
the conformance suite fails fast rather than producing vacuous equality results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a5fee9d4-3054-4018-8caa-31aa89d048f1
📒 Files selected for processing (6)
.github/workflows/napi-build.ymlpackages/sdk-node/package.jsonpackages/sdk-node/src/binding.cjspackages/sdk-node/src/index.cjspackages/sdk-node/src/index.jspackages/sdk-node/test/conformance.test.js
✅ Files skipped from review due to trivial changes (1)
- packages/sdk-node/package.json
…nd 3)
Extend ensureFixtureLedger() to fail closed on malformed fixtures, not
just missing directories. The previous check (existsSync of the dir)
let an empty or wrongly-copied fixture pass: both impls would then
deep-equal identical empty outputs, tautologically green.
New preconditions, all with seed-hint messages:
1. ledger.jsonl exists (canonical filename per packages/ledger/src/paths.ts).
2. ledger.jsonl is non-empty.
3. First line parses as JSON and matches LedgerLine shape (v:1 + known kind).
4. At least one kind:'turn' line is present so summary/hotspots/compare
have non-trivial input on both sides.
Manually exercised each failure path with RELAYBURN_SDK_NAPI_BUILT=1
against (a) empty dir, (b) empty file, (c) non-JSON content, and
(d) stamp-only fixture — each produced a distinct, actionable error.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… conflict) # Conflicts: # CHANGELOG.md
…I manifest-path # Conflicts: # CHANGELOG.md
…) instead of unused cross install
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wave 1 D2 of the Rust port (epic #240). Stands up the npm packaging side of the napi-rs deliverable so #247-a (parallel agent —
crates/relayburn-sdk-nodesource) has a target to publish into.Refs #247.
Summary
packages/sdk-node/umbrella + per-platform packages.@relayburn/sdk@2.0.0-pre.0resolves the right native binary viaoptionalDependencieson@relayburn/sdk-{darwin-arm64,darwin-x64,linux-arm64-gnu,linux-x64-gnu}(napi-rs's standard layout — same shape as@napi-rs/canvas,@swc/core, etc.). Win32 deferred to a follow-up.src/index.jsESM +src/index.cjs) re-exports the six verbs (ingest,summary,sessionCost,overhead,overheadTrim,hotspots) pluscompareand theLedgerconstructor — no behavior, just pass-through to./binding.js.src/index.d.tsmirrorspackages/sdk/index.d.tsbyte-for-byte modulobigintfor u64 token counts..github/workflows/napi-build.yml: builds prebuilt.nodefor darwin-arm64, darwin-x64, linux-x64-gnu, linux-arm64-gnu (latter viacross). Runs on PRs touchingcrates/relayburn-sdk{,-node}/**orpackages/sdk-node/**, on main pushes, and on workflow_dispatch. Publish step is a stub — full wiring lands in [Rust port] Lockstep 2.0 release (TS 1.x final + cutover + npm deprecate) #249 (Wave 3 cutover).test/conformance.test.js): deep-equals TS@relayburn/sdk@1.xvs the napi-rs umbrella across all six verbs against the existing fixture ledger. Skips whenRELAYBURN_SDK_NAPI_BUILTis unset (the default while [Rust port] relayburn-sdk-node + @relayburn/sdk 2.0 (napi-rs) #247-a is in flight). Once [Rust port] relayburn-sdk-node + @relayburn/sdk 2.0 (napi-rs) #247-a's bindings produce a real*.nodeartifact, flipping the env var innapi-build.ymlenables the gate.test/esbuild-smoke.test.js): writes a tiny entry that imports every named export from@relayburn/sdk, runs it through esbuild with native-binding externals, and asserts no errors / warnings. Catches resolution / bundling regressions for downstream embedders.pnpm-workspace.yaml: excludespackages/sdk-nodeso its@relayburn/sdk@2.0.0-pre.0doesn't shadowpackages/sdk@1.10.0duringworkspace:*resolution incli/mcp. CI installs sdk-node deps standalone viapnpm install --ignore-workspace.Coordination with #247-a
This PR does not touch
crates/relayburn-sdk-node/src/lib.rsorCargo.toml— that's #247-a's scope (parallel agent onrust-port/sdk-node-skeleton). I assumed the bindings will export the seven verb names listed above, matchingpackages/sdk/index.d.ts. If #247-a settles on different export names (e.g. snake_case from Rust → camelCase via napi-rs),packages/sdk-node/src/index.{js,cjs,d.ts}needs a one-line edit per verb.TODOs left for the #247-a-after-merge follow-up:
src/binding.{js,d.ts}stubs oncenapi buildruns cleanly against a real binding crate (the napi-rs CLI overwrites both files with generated output).RELAYBURN_SDK_NAPI_BUILT=1in.github/workflows/napi-build.ymlto enable the conformance gate..d.tssnapshot test +bigintboundary docs.nexttag.Test plan
napi-build.yml's build matrix (4 platforms; the build step may produce empty addons until [Rust port] relayburn-sdk-node + @relayburn/sdk 2.0 (napi-rs) #247-a lands its#[napi]exports — that's expected).pnpm install --frozen-lockfile && pnpm run build && pnpm run testat the workspace root passes (verifies the workspace exclusion didn't break the 1.x packages).cd packages/sdk-node && pnpm install --ignore-workspace && node --test test/passes locally — esbuild smoke test runs, conformance tests skip cleanly.cargo build --workspace && cargo test --workspacestill green.RELAYBURN_SDK_NAPI_BUILT=1, confirm conformance suite turns green on all four matrix legs.🤖 Generated with Claude Code